Skip to content

Make create-env-files.sh respect min python version supported - #1446

Closed
mhucka wants to merge 1 commit into
quantumlib:mainfrom
mhucka:create-env-with-min-python
Closed

Make create-env-files.sh respect min python version supported#1446
mhucka wants to merge 1 commit into
quantumlib:mainfrom
mhucka:create-env-with-min-python

Conversation

@mhucka

@mhucka mhucka commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

This makes dev_tools/requirements/create-env-files.sh get the minimum version of Python the project supports from pyproject.toml, and use that as a constraint passed to uv.

This makes `dev_tools/requirements/create-env-files.sh` get the minimum
version of Python the project supports from pyproject.toml, and use that
as a constraint passed to `uv`.
@mhucka
mhucka marked this pull request as ready for review August 18, 2026 20:18
@mhucka
mhucka requested a review from pavoljuhas August 18, 2026 20:18

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the dev_tools/requirements/create-env-files.sh script to dynamically determine the minimum Python version from pyproject.toml and introduces a new -p / --min-python option to override this default. Feedback on these changes highlights three key areas for improvement: handling scenarios where the script is run outside of a git repository to avoid cryptic failures, making the inline Python parser more robust against missing keys in pyproject.toml to prevent unhandled exceptions, and refactoring the argument parsing loop so that it does not prematurely terminate when encountering unrecognized options.

Comment on lines +20 to +22
script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P)
repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null)
cd "${repo_dir}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the script is run outside of a git repository (for example, from a source archive/tarball) or if git is not installed, repo_dir will be empty. Because set -o errexit is enabled, the subsequent cd "${repo_dir}" will fail with a cryptic error and terminate the script immediately. We should check if repo_dir is empty and provide a clear, helpful error message.

Suggested change
script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P)
repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null)
cd "${repo_dir}"
script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P)
repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null)
if [[ -z "${repo_dir}" ]]; then
echo "Error: Failed to determine repository root. Are you in a git repository?" >&2
exit 1
fi
cd "${repo_dir}"

Comment on lines +24 to +30
default_min_python=$(python3 -c 'import re
s = open("pyproject.toml").read()
m = re.search(r"requires-python.*?>=?(\d+\.\d+)", s)
if not m:
m = re.search(r"target-version.*?py(\d)(\d+)", s)
print(m.group(1) if "requires-python" in m.re.pattern else m.expand(r"\1.\2"))
' 2>/dev/null || echo "3.10")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If pyproject.toml exists but contains neither requires-python nor target-version, the python script will raise an unhandled AttributeError when trying to access m.re or m.expand on a None object. While the shell script catches this with || echo "3.10", relying on silent python crashes is fragile and makes debugging harder. We should handle the None case and potential exceptions gracefully within the Python snippet.

Suggested change
default_min_python=$(python3 -c 'import re
s = open("pyproject.toml").read()
m = re.search(r"requires-python.*?>=?(\d+\.\d+)", s)
if not m:
m = re.search(r"target-version.*?py(\d)(\d+)", s)
print(m.group(1) if "requires-python" in m.re.pattern else m.expand(r"\1.\2"))
' 2>/dev/null || echo "3.10")
default_min_python=$(python3 -c 'import re
try:
s = open("pyproject.toml").read()
m = re.search(r"requires-python.*?>=?(\d+\.\d+)", s)
if m:
print(m.group(1))
else:
m = re.search(r"target-version.*?py(\d)(\d+)", s)
print(m.expand(r"\1.\2") if m else "3.10")
except Exception:
print("3.10")
' 2>/dev/null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slop and Bloat Gemini, Yuck!

Comment on lines 48 to 62
while [[ $# -gt 0 ]]; do
case "${1}" in
-h|--help) echo "${usage}"; exit 0 ;;
-p|--min-python|--python-version)
if [[ $# -lt 2 || -z "${2:-}" ]]; then
echo "Error: option '${1}' requires an argument" >&2
echo "${usage}" >&2
exit 1
fi
min_python=("--python-version" "${2}")
shift 2
;;
*) break ;;
esac
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current argument parsing loop terminates on the first unrecognized option (using *) break ;;). If a user passes any other options (such as --upgrade or --no-cache) before -p or --min-python, the script will stop parsing arguments, leaving -p unparsed and passing it directly to uv pip compile, which will fail because uv does not recognize -p.

We can fix this by collecting unrecognized options into an array and resetting the positional parameters ($@) at the end of the loop.

Suggested change
while [[ $# -gt 0 ]]; do
case "${1}" in
-h|--help) echo "${usage}"; exit 0 ;;
-p|--min-python|--python-version)
if [[ $# -lt 2 || -z "${2:-}" ]]; then
echo "Error: option '${1}' requires an argument" >&2
echo "${usage}" >&2
exit 1
fi
min_python=("--python-version" "${2}")
shift 2
;;
*) break ;;
esac
done
uv_options=()
while [[ $# -gt 0 ]]; do
case "${1}" in
-h|--help) echo "${usage}"; exit 0 ;;
-p|--min-python|--python-version)
if [[ $# -lt 2 || -z "${2:-}" ]]; then
echo "Error: option '${1}' requires an argument" >&2
echo "${usage}" >&2
exit 1
fi
min_python=("--python-version" "${2}")
shift 2
;;
*)
uv_options+=("${1}")
shift
;;
esac
done
set -- "${uv_options[@]}"

@pavoljuhas pavoljuhas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please set python-version via pyproject.toml instead. Most of the changes in this PR can be then reverted.

declare -r usage="Usage: ${0} [-h] [UV_OPTIONS]
# Go to the top of the local TFQ git tree. Do it early in case this fails.
script_dir=$(CDPATH="" cd -- "$(dirname -- "${0}")" && pwd -P)
repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should keep the stderr message so user gets a clue what is going on if the script exits here on git-rev-parse failure.

Suggested change
repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null)
repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel)

repo_dir=$(git -C "${script_dir}" rev-parse --show-toplevel 2>/dev/null)
cd "${repo_dir}"

default_min_python=$(python3 -c 'import re

@pavoljuhas pavoljuhas Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way too complicated and as is it extracts the black formatter setting which may be unrelated to the actual supported version. A better way would be to grep setup.py for python_requires, but I feel even that would be more fragile and more laborious to maintain in a long term than to just set

default_min_python=3.10

and bump it up when the minimum required version goes up.

Add: not needed at all per #1446 (comment)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setup.py is going away in another PR, which is why this doesn't look in setup.py


Options:
-h Show this help message and exit
-h Show this help message and exit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
-h Show this help message and exit
-h, --help Show this help message and exit

while [[ $# -gt 0 ]]; do
case "${1}" in
-h|--help) echo "${usage}"; exit 0 ;;
-p|--min-python|--python-version)

@pavoljuhas pavoljuhas Aug 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is shadowing the uv-pip -p, --python option which would be ignored if specified first, but processed if placed as a later argument. In addition, user would not be able to pass --python-version=X.Y.Z because uv does not accept that option multiple times.

I recommend to add the python-version setting to pyproject.toml instead and drop the custom -p, --min-python option here altogether:

diff --git a/pyproject.toml b/pyproject.toml
index 72ba61d..b8de85d 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -73,4 +73,5 @@ follow_untyped_imports = true
 [tool.uv.pip]
 universal = true
 generate-hashes = true
 custom-compile-command = "dev_tools/requirements/create-env-files.sh"
+python-version = "3.10"

# ~~~~ Generate basic requirements files ~~~~

uv pip compile "$@" \
uv pip compile "${min_python[@]}" "$@" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and below can be reverted after setting python-version in pyproject.toml.

@mhucka

mhucka commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator Author

Please set python-version via pyproject.toml instead. Most of the changes in this PR can be then reverted.

Good point. There's another PR that will move setup.py settings to pyproject.toml. I'll close this one and then check if any changes are even needed after that, because the python version set in pyproject.toml may be all that's needed.

@mhucka mhucka closed this Aug 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants